Skip to content

Add getQueueUrls and SQS queue name#11151

Merged
kaiyan-sheng merged 17 commits intoelastic:masterfrom
kaiyan-sheng:fix_s3_sqs
Mar 13, 2019
Merged

Add getQueueUrls and SQS queue name#11151
kaiyan-sheng merged 17 commits intoelastic:masterfrom
kaiyan-sheng:fix_s3_sqs

Conversation

@kaiyan-sheng
Copy link
Copy Markdown
Contributor

@kaiyan-sheng kaiyan-sheng commented Mar 8, 2019

While trying to create Kibana Dashboards for SQS, I realized there are two bugs in the code:

  1. sqs metricset is reporting metrics without queue name, which is needed for dashboards.

  2. When there are more than 1 queue in the same region, events get overwritten and only the last queue get reported. This bug is fixed in this PR by querying ListQueues first to get a list of queues and then loop through each queue name to construct and report events.

@kaiyan-sheng kaiyan-sheng requested review from a team as code owners March 8, 2019 06:18
@kaiyan-sheng kaiyan-sheng changed the title Fix s3 metricset timestamp and sqs queue name Fix s3 metricset timestamp check and sqs queue name Mar 8, 2019
@kaiyan-sheng kaiyan-sheng self-assigned this Mar 8, 2019
@kaiyan-sheng kaiyan-sheng added Metricbeat Metricbeat Team:Integrations Label for the Integrations team in progress Pull request is currently in progress. labels Mar 8, 2019
Comment thread x-pack/metricbeat/module/aws/sqs/sqs.go Outdated
Comment thread x-pack/metricbeat/module/aws/sqs/sqs.go Outdated
@ruflin
Copy link
Copy Markdown
Contributor

ruflin commented Mar 11, 2019

Will this also need a backport?

Copy link
Copy Markdown
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Do you know why sometimes they don't have a timestamp?

How are the scaled_float changes related to the PR description? Perhaps easier to review if multiple PR's?

Comment thread x-pack/metricbeat/module/aws/s3_daily_storage/s3_daily_storage.go Outdated
@kaiyan-sheng
Copy link
Copy Markdown
Contributor Author

Will this also need a backport?

If we have 7.x branch opened, then this will need a backport to 7.x. Because the changes are not related to ec2 metricset, so no backport to 7.0.

Comment thread x-pack/metricbeat/module/aws/utils.go Outdated
@kaiyan-sheng kaiyan-sheng changed the title Fix s3 metricset timestamp check and sqs queue name Add getQueueUrls and SQS queue name Mar 11, 2019
Comment thread x-pack/metricbeat/module/aws/sqs/sqs.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I would advice to not have for loops inside for loops especially if they contain quite a bit of code and instead use functions/methods. This gives also the opportunity to give some nice names to the functions and potentially have docs there. This will also help with testability.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I don't know in this case if helped or not :-) I moved the for loop into createSQSEvents. Thanks!

Comment thread x-pack/metricbeat/module/aws/sqs/sqs.go Outdated
@ruflin
Copy link
Copy Markdown
Contributor

ruflin commented Mar 13, 2019

This will need a rebase.

description: >
TThe number of messages in the queue that are delayed and not available for reading immediately.
- name: messages.not_visible
- name: message.not_visible
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this means we don't check today if the fields are documented? I hope we get this coverage when we have the new "data tests".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! Will track this in #11226

Copy link
Copy Markdown
Contributor

@ruflin ruflin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I think this does not need a changelog as we didn't release it yet?

@kaiyan-sheng
Copy link
Copy Markdown
Contributor Author

Yeah no changelog :-)

@kaiyan-sheng kaiyan-sheng merged commit a6a21f4 into elastic:master Mar 13, 2019
@kaiyan-sheng kaiyan-sheng deleted the fix_s3_sqs branch March 13, 2019 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Metricbeat Metricbeat review Team:Integrations Label for the Integrations team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants